buffer: improve fill(number) performance by mscdex · Pull Request #31489 · nodejs/node

mscdex · 2020-01-24T07:48:49Z

Uses the native typedArray.fill() when filling with a number instead of calling into the binding fill function.

Benchmark results:

                                                                            confidence improvement accuracy (*)   (**)  (***)
 buffers/buffer-fill.js n=20000 size=65536 type='fill("")'                        ***      4.72 %       ±0.90% ±1.20% ±1.57%
 buffers/buffer-fill.js n=20000 size=65536 type='fill("t", "utf8")'               ***      3.76 %       ±1.00% ±1.34% ±1.74%
 buffers/buffer-fill.js n=20000 size=65536 type='fill("t", 0, "utf8")'            ***      4.30 %       ±0.74% ±0.99% ±1.29%
 buffers/buffer-fill.js n=20000 size=65536 type='fill("t", 0)'                    ***      5.56 %       ±0.90% ±1.20% ±1.56%
 buffers/buffer-fill.js n=20000 size=65536 type='fill("t")'                       ***      4.51 %       ±1.01% ±1.35% ±1.77%
 buffers/buffer-fill.js n=20000 size=65536 type='fill("test")'                             0.18 %       ±0.50% ±0.67% ±0.87%
 buffers/buffer-fill.js n=20000 size=65536 type='fill(0)'                         ***      4.10 %       ±0.84% ±1.12% ±1.46%
 buffers/buffer-fill.js n=20000 size=65536 type='fill(100)'                       ***      4.75 %       ±0.97% ±1.30% ±1.69%
 buffers/buffer-fill.js n=20000 size=65536 type='fill(400)'                       ***      5.17 %       ±1.12% ±1.50% ±1.95%
 buffers/buffer-fill.js n=20000 size=65536 type='fill(Buffer.alloc(1), 0)'                -0.50 %       ±0.66% ±0.88% ±1.15%
 buffers/buffer-fill.js n=20000 size=8192 type='fill("")'                         ***     17.23 %       ±1.65% ±2.20% ±2.86%
 buffers/buffer-fill.js n=20000 size=8192 type='fill("t", "utf8")'                ***     12.32 %       ±1.62% ±2.18% ±2.87%
 buffers/buffer-fill.js n=20000 size=8192 type='fill("t", 0, "utf8")'             ***     13.12 %       ±2.05% ±2.73% ±3.56%
 buffers/buffer-fill.js n=20000 size=8192 type='fill("t", 0)'                     ***     18.16 %       ±1.00% ±1.33% ±1.74%
 buffers/buffer-fill.js n=20000 size=8192 type='fill("t")'                        ***     12.18 %       ±1.64% ±2.18% ±2.86%
 buffers/buffer-fill.js n=20000 size=8192 type='fill("test")'                             -0.34 %       ±1.20% ±1.59% ±2.08%
 buffers/buffer-fill.js n=20000 size=8192 type='fill(0)'                          ***     26.16 %       ±2.99% ±3.99% ±5.21%
 buffers/buffer-fill.js n=20000 size=8192 type='fill(100)'                        ***     28.79 %       ±1.14% ±1.52% ±1.98%
 buffers/buffer-fill.js n=20000 size=8192 type='fill(400)'                        ***     28.91 %       ±1.20% ±1.60% ±2.08%
 buffers/buffer-fill.js n=20000 size=8192 type='fill(Buffer.alloc(1), 0)'                 -0.40 %       ±1.31% ±1.75% ±2.28%

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

nodejs-github-bot · 2020-01-24T07:49:50Z

CI: https://ci.nodejs.org/job/node-test-pull-request/28603/

bnoordhuis

LGTM % comments.

bnoordhuis · 2020-01-24T09:00:03Z

lib/buffer.js

Suggested change

const TypedArrayPrototype = ObjectGetPrototypeOf(Uint8Array.prototype);

const Uint8ArrayPrototype = ObjectGetPrototypeOf(Uint8Array.prototype);

Uint8Array.prototype.constructor.name is Uint8Array, not TypedArray.

This line is correct as-is. The prototype of Uint8Array.prototype is the %TypedArray%.prototype.

@devsnek is correct, this is the prototype of the prototype, which is TypedArray. The same thing is done in internal/util/types.js.

lib/buffer.js

src/node_buffer.cc

nodejs-github-bot · 2020-01-24T11:12:26Z

CI: https://ci.nodejs.org/job/node-test-pull-request/28604/

lib/buffer.js

targos · 2020-01-24T15:10:18Z

I think we could also directly add TypedArray and its prototype to the primordials. I'll do that after this PR.

nodejs-github-bot · 2020-01-24T15:19:24Z

CI: https://ci.nodejs.org/job/node-test-pull-request/28607/

nodejs-github-bot · 2020-01-25T23:03:14Z

CI: https://ci.nodejs.org/job/node-test-pull-request/28668/

bnoordhuis · 2020-01-26T01:21:22Z

lib/buffer.js

Future enhancement: res < 0 is sometimes a coercion-to-nan, sometimes not because bindingFill() returns either undefined or a number.

Changing Fill() in node_buffer.cc to always return a number is probably beneficial for performance.

nodejs-github-bot · 2020-01-26T02:01:43Z

CI: https://ci.nodejs.org/job/node-test-pull-request/28673/

PR-URL: nodejs#31489 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

nodejs-github-bot · 2020-01-27T01:05:19Z

CI: https://ci.nodejs.org/job/node-test-pull-request/28705/

Trott · 2020-01-27T02:34:39Z

This seems to have landed without first getting a clean CI run.

Trott · 2020-01-27T02:59:17Z

This seems to have landed without first getting a clean CI run.

Although if there was an impressive-benchmarks-mean-you-can-land-without-a-clean-CI rule, this would definitely qualify. 😍

PR-URL: #31489 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

mscdex added buffer Issues and PRs related to the buffer subsystem. performance Issues and PRs related to the performance of Node.js. labels Jan 24, 2020

nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Jan 24, 2020

bnoordhuis approved these changes Jan 24, 2020

View reviewed changes

mscdex force-pushed the buffer-improve-fill-number-perf branch from 039c099 to aa45188 Compare January 24, 2020 11:08

addaleax approved these changes Jan 24, 2020

View reviewed changes

BridgeAR approved these changes Jan 24, 2020

View reviewed changes

BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 24, 2020

cjihrig approved these changes Jan 24, 2020

View reviewed changes

targos reviewed Jan 24, 2020

View reviewed changes

lib/buffer.js Outdated Show resolved Hide resolved

targos reviewed Jan 24, 2020

View reviewed changes

lib/buffer.js Outdated Show resolved Hide resolved

mscdex force-pushed the buffer-improve-fill-number-perf branch from aa45188 to ac9de4f Compare January 24, 2020 15:18

Trott approved these changes Jan 25, 2020

View reviewed changes

bnoordhuis reviewed Jan 26, 2020

View reviewed changes

mscdex force-pushed the buffer-improve-fill-number-perf branch from ac9de4f to 59cba9a Compare January 27, 2020 00:17

mscdex merged commit 59cba9a into nodejs:master Jan 27, 2020

mscdex deleted the buffer-improve-fill-number-perf branch January 27, 2020 00:21

codebytere mentioned this pull request Feb 17, 2020

v13.9.0 proposal #31837

Merged

codebytere mentioned this pull request Mar 17, 2020

v12.16.2 proposal #32313

Merged

	const TypedArrayPrototype = ObjectGetPrototypeOf(Uint8Array.prototype);
	const Uint8ArrayPrototype = ObjectGetPrototypeOf(Uint8Array.prototype);

Uh oh!

Conversation

mscdex commented Jan 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

nodejs-github-bot commented Jan 24, 2020

Uh oh!

bnoordhuis left a comment

Choose a reason for hiding this comment

Uh oh!

bnoordhuis Jan 24, 2020

Choose a reason for hiding this comment

Uh oh!

devsnek Jan 24, 2020

Choose a reason for hiding this comment

Uh oh!

mscdex Jan 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

nodejs-github-bot commented Jan 24, 2020

Uh oh!

Uh oh!

Uh oh!

targos commented Jan 24, 2020

Uh oh!

nodejs-github-bot commented Jan 24, 2020

Uh oh!

nodejs-github-bot commented Jan 25, 2020

Uh oh!

bnoordhuis Jan 26, 2020

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Jan 26, 2020

Uh oh!

nodejs-github-bot commented Jan 27, 2020

Uh oh!

Trott commented Jan 27, 2020

Uh oh!

Trott commented Jan 27, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

mscdex commented Jan 24, 2020 •

edited

Loading

mscdex Jan 24, 2020 •

edited

Loading